Open
Conversation
- Add module-level native_uuid property (default=False, matching pyodbc) - Add per-connection native_uuid override via connect(native_uuid=True/False) - Connection-level setting takes precedence over module-level - Snapshot native_uuid at execute() time for consistency within result sets - Return uppercase UUID strings when native_uuid=False (pyodbc compat) - Extract _compute_uuid_str_indices() helper to eliminate code duplication - Move uuid import to module-level in row.py (avoid per-row lazy import) - Fix Row.__init__ signature in .pyi stub to match implementation - Remove duplicate DDBCSQLDescribeCol call in execute() - Add comprehensive tests for both module-level and per-connection control - Thread-safe settings via _settings_lock
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.hpp: 58.8%
mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 67.8%
mssql_python.pybind.ddbc_bindings.cpp: 69.7%
mssql_python.row.py: 70.5%
mssql_python.pybind.connection.connection.cpp: 75.3%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 85.2%
mssql_python.cursor.py: 85.6%🔗 Quick Links
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a native_uuid switch to control how SQL Server UNIQUEIDENTIFIER columns are returned: either as uuid.UUID objects or as pyodbc-compatible strings, configurable at module and per-connection scope.
Changes:
- Introduces a global
native_uuidsetting (module property +Settings) and a per-connectionnative_uuidoverride onconnect()/Connection. - Updates
Cursor/Rowto snapshot the effective UUID mode atexecute()time and efficiently convert UUID columns to uppercase strings whennative_uuid=False. - Expands/adjusts type stubs and adds extensive coverage for module-level and per-connection behaviors in tests.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
mssql_python/helpers.py |
Adds Settings.native_uuid and sets the default behavior. |
mssql_python/__init__.py |
Exposes native_uuid as a module-level property with validation. |
mssql_python/connection.py |
Adds native_uuid per-connection override + validation and stores it on the connection. |
mssql_python/db_connection.py |
Threads native_uuid through the public connect() helper + docstring updates. |
mssql_python/cursor.py |
Precomputes UUID column indices per result set and passes them to Row to avoid per-cell checks. |
mssql_python/row.py |
Implements UUID-to-uppercase-string conversion for selected columns when requested. |
mssql_python/mssql_python.pyi |
Updates public type hints for Row, Connection, and connect() to include native_uuid. |
tests/test_001_globals.py |
Adds global default/validation/consistency/threading tests for native_uuid. |
tests/test_004_cursor.py |
Adds end-to-end cursor tests for native UUID handling, snapshot semantics, and per-connection overrides. |
Comments suppressed due to low confidence (1)
tests/test_001_globals.py:816
- This thread-safety test uses an unbounded
Queueand continuouslyput()s into it from multiple tight loops, but the queue is never drained/consumed. This can cause runaway memory growth and slow/flaky CI. Consider removing the queue entirely (it isn’t used for assertions), or bounding it (maxsize) and/or only recording aggregated counts with a small sleep/backoff in the loops.
original = mssql_python.native_uuid
results_queue = queue.Queue()
stop_event = threading.Event()
errors = []
def writer_thread():
"""Toggle native_uuid between True and False."""
try:
while not stop_event.is_set():
mssql_python.native_uuid = True
mssql_python.native_uuid = False
results_queue.put(("write", True))
except Exception as e:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+371
to
+373
| # or str (False). Default False matches pyodbc behavior for seamless migration. | ||
| # Set to True to return native uuid.UUID objects. | ||
| self.native_uuid: bool = False |
There was a problem hiding this comment.
native_uuid is initialized to False, which changes the default UNIQUEIDENTIFIER result type from uuid.UUID to str for existing users (previously UUIDs were always returned as uuid.UUID based on the type mapping). If this is intentional, it should be treated as a breaking change (documented prominently and aligned with the package’s versioning/release notes); otherwise, consider defaulting to True and letting users opt into pyodbc-style strings via native_uuid=False.
Suggested change
| # or str (False). Default False matches pyodbc behavior for seamless migration. | |
| # Set to True to return native uuid.UUID objects. | |
| self.native_uuid: bool = False | |
| # or str (False). Default True preserves the historical behavior of returning | |
| # native uuid.UUID objects. Set to False to opt into pyodbc-style string results. | |
| self.native_uuid: bool = True |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
This pull request introduces support for native UUID handling in the
mssql_pythonpackage, allowing users to control whether SQL ServerUNIQUEIDENTIFIERcolumns are returned asuuid.UUIDobjects or as strings. The feature is configurable at both the module and per-connection levels, enabling incremental adoption and backward compatibility with pyodbc-style string UUIDs. The implementation ensures efficient conversion with minimal runtime overhead and updates the public API, documentation, and type hints accordingly.Native UUID Handling
native_uuidsetting to the global settings (Settingsclass inhelpers.py) and exposed it as a property on the module, allowing users to control UUID handling globally. (mssql_python/helpers.py,mssql_python/__init__.py)ConnectionandconnectAPI to accept anative_uuidparameter, enabling per-connection overrides of the global setting. Updated docstrings and type hints to document this parameter. (mssql_python/connection.py,mssql_python/db_connection.py,mssql_python/mssql_python.pyi)Cursor and Row Conversion Logic
Cursorclass to determine the effectivenative_uuidsetting and efficiently precompute which columns require conversion to string, minimizing per-row overhead. (mssql_python/cursor.py)Rowclass to accept a list of UUID column indices and convert those columns to uppercase strings only whennative_uuid=False, preserving pyodbc compatibility and allowing seamless migration. (mssql_python/row.py,mssql_python/mssql_python.pyi)Testing and Minor Updates
native_uuidsymbol. (tests/test_001_globals.py)tests/test_001_globals.py)These changes provide a robust and flexible way for users to opt in to native UUID support, with clear migration paths and minimal performance impact.